Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kidroca/onyx cache cleanup #79

Merged
merged 5 commits into from
Jun 8, 2021

Conversation

kidroca
Copy link
Contributor

@kidroca kidroca commented Jun 8, 2021

@marcaaron @Jag96 @roryabraham

Details

Address some NAB leftovers from #76

Related Issues

#63, Expensify/App#2762

Automated Tests

Cache tests were updated to require AsyncStorage in the before each block

Linked PRs

Expensify/App#3423

@kidroca kidroca requested a review from a team as a code owner June 8, 2021 12:38
@MelvinBot MelvinBot requested review from jasperhuangg and removed request for a team June 8, 2021 12:38
@kidroca
Copy link
Contributor Author

kidroca commented Jun 8, 2021

I found an issue:

// Merge original data to cache
cache.merge(collection);
// Optimistically inform collection subscribers
keysChanged(collectionKey, collection);

cache.merge does not update available storage keys

/**
* Deep merge data to cache, any non existing keys will be crated
* @param {Record<string, *>} data - a map of (cache) key - values
*/
merge(data) {
this.storageMap = lodashMerge({}, this.storageMap, data);
}

  1. A request to fetch user reports and IOUs completes before LNH and chats are rendered
  2. It merges data to storage and cache, but cache does not update available keys from getAllKeys (bug)
  3. Component tries to connect with Onyx
  4. It first getAllKeys and see the key does not exist in storage (bug)
  5. And gives the subscriber null as the initial value instead of reading from storage

This also reviews a small secondary issue - data can be merged to cache even when there are no subscribers for a given object
While we want to clear cache items when there are no subscribers for the data

@kidroca
Copy link
Contributor Author

kidroca commented Jun 8, 2021

Updated. Added a fix for the above issue

@kidroca
Copy link
Contributor Author

kidroca commented Jun 8, 2021

Regarding

This also reviews a small secondary issue - data can be merged to cache even when there are no subscribers for a given object
While we want to clear cache items when there are no subscribers for the data

The above goal can be achieved if we skip merging keys that are not already in cache
If a key is not in cache - no one is subscribed for it, we should skip adding it to cache, because we want to keep only data that's used on screen, but at the same time we should still update storageKeys so that we keep it up to date and know the key is available

But this is also the weak point of this strategy as we know for a fact that the collection is going to be used shortly, and if we don't store it in cache it will be read from file
The collection is literally retrieved less than half a second after the mergeCollection call and because it's available in cache I don't have to read 10 collection items (chats) from file

Should I update this with the logic to skip the collection keys that do not already exist in cache?


Here's what actually happens for clean startup in practice

  1. A lot of information is fetched - LHN and chats are not yet rendered
  2. Fetching reports is completed before LHN and Chat have started to render
  3. A call to mergeCollection is made - no withOnyx components use the data yet
  4. LHN and Chat start to render
  5. At this point a component withOnyx makes a connection for the above data
  6. As this is the first connection data is read from storage, even though it was just fetched

@marcaaron
Copy link
Contributor

This also reviews a small secondary issue - data can be merged to cache even when there are no subscribers for a given object While we want to clear cache items when there are no subscribers for the data

Not sure I entirely follow the explanation above or the consequences. But I would maybe just err on the side of caution and add the values to the cache whether they have active subscribers or not 🤷‍♂️

@marcaaron marcaaron merged commit ebde257 into Expensify:master Jun 8, 2021
@kidroca kidroca deleted the kidroca/onyx-cache-cleanup branch June 8, 2021 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants